Desugar IfLet* expr to match #3064

dkm · 2024-06-23T21:58:08Z

Replace the "regular" AST->HIR lowering for IfLet* with a desugaring
into a MatchExpr.
Desugar a simple if let:

   if let Some(y) = some_value {
     bar();
   }

into:

   match some_value {
     Some(y) => {bar();},
     _ => ()
   }

Same applies for IfLetExprConseqElse (if let with an else block).

Desugar:

   if let Some(y) = some_value {
     bar();
   } else {
     baz();
   }

into:

   match some_value {
     Some(y) => {bar();},
     _ => {baz();}
   }

Fixes #1177

gcc/rust/hir/rust-ast-lower-block.h

dkm · 2024-09-05T20:08:47Z

@CohenArthur I think this is now ready for review (but of course, no hurry).

dkm · 2024-09-06T15:44:06Z

Looks like the alpine builder had a network issue: [7] Could not connect to server (Failed to connect to static.crates.io port 443 after 0 ms: Could not connect to server)

P-E-P

Looks great

gcc/rust/hir/rust-ast-lower.cc

dkm · 2024-09-10T21:17:24Z

Also, I'm not really sure how this impacts error reporting... What if we report an error after the HIR lowering. Do we already have a mechanism to make sure we're not reporting something confusing (e.g. a diagnostic mentioning a match error pointing to the "if let").

P-E-P · 2024-09-11T08:56:16Z

Also, I'm not really sure how this impacts error reporting... What if we report an error after the HIR lowering. Do we already have a mechanism to make sure we're not reporting something confusing (e.g. a diagnostic mentioning a match error pointing to the "if let").

Mmh good point, I think there is no mention of if let at this stage in the compile process, but the location should still point to the initial if let statement, if an error arise it should be correctly annotated anyway.

dkm · 2024-09-11T10:07:53Z

Also, I'm not really sure how this impacts error reporting... What if we report an error after the HIR lowering. Do we already have a mechanism to make sure we're not reporting something confusing (e.g. a diagnostic mentioning a match error pointing to the "if let").

Mmh good point, I think there is no mention of if let at this stage in the compile process, but the location should still point to the initial if let statement, if an error arise it should be correctly annotated anyway.

Sure, but I'm afraid we may produce nonsensical diagnostic at some point... I'll try to push the compiler to emit errors around this... But maybe it's not possible by construction... at least for the moment. But we probably need something to identify synthetic construction from the ones that come from the source.

dkm · 2024-09-12T13:09:45Z

Something like:

dkm · 2024-10-06T17:13:51Z

@CohenArthur I thought the FAIL that this PR has in nr2 test was caused by my change... but it seems it's caused by my new test case.

crab1: internal compiler error: in visit, at rust/resolve/rust-late-name-resolver-2.0.cc:207
0x259fde4 Rust::Resolver2_0::Late::visit(Rust::AST::PathInExpression&)
        ../../gcc/rust/resolve/rust-late-name-resolver-2.0.cc:207
0x237a889 Rust::AST::DefaultASTVisitor::visit(Rust::AST::TupleStructPattern&)                                                                                                 
        ../../gcc/rust/ast/rust-ast-visitor.cc:1231                                                                                                                           
0x2357099 Rust::AST::TupleStructPattern::accept_vis(Rust::AST::ASTVisitor&)       
        ../../gcc/rust/ast/rust-pattern.cc:487
0x237dc25 void Rust::AST::DefaultASTVisitor::visit<Rust::AST::Pattern>(std::unique_ptr<Rust::AST::Pattern, std::default_delete<Rust::AST::Pattern> >&)
        ../../gcc/rust/ast/rust-ast-visitor.h:409                          
0x2377cc0 Rust::AST::DefaultASTVisitor::visit(Rust::AST::IfLetExpr&)                                                                                                          
        ../../gcc/rust/ast/rust-ast-visitor.cc:623
0x2377d63 Rust::AST::DefaultASTVisitor::visit(Rust::AST::IfLetExprConseqElse&)
        ../../gcc/rust/ast/rust-ast-visitor.cc:631          
0x2277c51 Rust::AST::IfLetExprConseqElse::accept_vis(Rust::AST::ASTVisitor&)
        ../../gcc/rust/ast/rust-ast.cc:4645 
0x237c2d9 void Rust::AST::DefaultASTVisitor::visit<Rust::AST::Expr>(Rust::AST::Expr&)
        ../../gcc/rust/ast/rust-ast-visitor.h:405
0x23773bb Rust::AST::DefaultASTVisitor::visit(Rust::AST::BlockExpr&)
        ../../gcc/rust/ast/rust-ast-visitor.cc:461
0x25803a4 operator()

In :

void
Late::visit (AST::PathInExpression &expr)
{
  // TODO: How do we have a nice error with `can't capture dynamic environment
  // in a function item` error here?
  // do we emit it in `get<Namespace::Labels>`?

  rust_debug ("[ARTHUR]: %s", expr.as_simple_path ().as_string ().c_str ());
  auto value = ctx.values.resolve_path (expr.get_segments ());
  if (!value.has_value ())
    rust_unreachable (); // Should have been resolved earlier

Test case :

enum MyOption {
    Some(i32),
    None,
}

pub fn toto(i : MyOption) -> i32 {
    if let MyOption::Some(v) = i {
        v
    } else {
        23i32
    }
}

Seen in https://rust.godbolt.org/z/1dYfWGY5T

dkm · 2024-10-15T21:04:19Z

@P-E-P do you know if there's a way to mark a test XFAIL only for a given target (nr2)? I don't think I can debug this issue as I know nearly nothing about the name resolution in gccrs, but I think this change could go in as it's not adding any issue, it's simply testing something that's currently already broken in nr2.

P-E-P · 2024-10-15T22:10:19Z

@P-E-P do you know if there's a way to mark a test XFAIL only for a given target (nr2)? I don't think I can debug this issue as I know nearly nothing about the name resolution in gccrs, but I think this change could go in as it's not adding any issue, it's simply testing something that's currently already broken in nr2.

There is an exclude file containing a list of test that do not pass with nr2 here https://github.com/Rust-GCC/gccrs/blob/master/gcc%2Ftestsuite%2Frust%2Fcompile%2Fnr2%2Fexclude

You can add your failing test, we'll remove it from the list when nr2 will be more complete.

dkm · 2024-10-16T08:48:47Z

You can add your failing test, we'll remove it from the list when nr2 will be more complete.

oh nice, thanks!

P-E-P

Looks great!

Replace the "regular" AST->HIR lowering for IfLet* with a desugaring into a MatchExpr. Desugar a simple if let: if let Some(y) = some_value { bar(); } into: match some_value { Some(y) => {bar();}, _ => () } Same applies for IfLetExprConseqElse (if let with an else block). Desugar: if let Some(y) = some_value { bar(); } else { baz(); } into: match some_value { Some(y) => {bar();}, _ => {baz();} } Fixes #1177 gcc/rust/ChangeLog: * backend/rust-compile-block.h: Adjust after removal of HIR::IfLetExpr and HIR::IfLetExprConseqElse. * backend/rust-compile-expr.h: Likewise. * checks/errors/borrowck/rust-bir-builder-expr-stmt.cc (ExprStmtBuilder::visit): Likewise. * checks/errors/borrowck/rust-bir-builder-expr-stmt.h: Likewise. * checks/errors/borrowck/rust-bir-builder-lazyboolexpr.h: Likewise. * checks/errors/borrowck/rust-bir-builder-struct.h: Likewise. * checks/errors/borrowck/rust-function-collector.h: Likewise. * checks/errors/privacy/rust-privacy-reporter.cc (PrivacyReporter::visit): Likewise. * checks/errors/privacy/rust-privacy-reporter.h: Likewise. * checks/errors/rust-const-checker.cc (ConstChecker::visit): Likewise. * checks/errors/rust-const-checker.h: Likewise. * checks/errors/rust-unsafe-checker.cc (UnsafeChecker::visit): Likewise. * checks/errors/rust-unsafe-checker.h: Likewise. * hir/rust-ast-lower-block.h (ASTLoweringIfLetBlock::translate): Change return type. * hir/rust-ast-lower.cc (ASTLoweringIfLetBlock::desugar_iflet): New. (ASTLoweringIfLetBlock::visit(AST::IfLetExpr &)): Adjust and use desugar_iflet. * hir/rust-ast-lower.h: Add comment. * hir/rust-hir-dump.cc (Dump::do_ifletexpr): Remove. (Dump::visit(IfLetExpr&)): Remove. (Dump::visit(IfLetExprConseqElse&)): Remove. * hir/rust-hir-dump.h (Dump::do_ifletexpr): Remove. (Dump::visit(IfLetExpr&)): Remove. (Dump::visit(IfLetExprConseqElse&)): Remove. * hir/tree/rust-hir-expr.h (class IfLetExpr): Remove. (class IfLetExprConseqElse): Remove. * hir/tree/rust-hir-full-decls.h (class IfLetExpr): Remove. (class IfLetExprConseqElse): Remove. * hir/tree/rust-hir-visitor.h: Adjust after removal of HIR::IfLetExpr and HIR::IfLetExprConseqElse. * hir/tree/rust-hir.cc (IfLetExpr::as_string): Remove. (IfLetExprConseqElse::as_string): Remove. (IfLetExpr::accept_vis): Remove. (IfLetExprConseqElse::accept_vis): Remove. * hir/tree/rust-hir.h: Adjust after removal of HIR::IfLetExpr and HIR::IfLetExprConseqElse. * typecheck/rust-hir-type-check-expr.cc (TypeCheckExpr::visit): Likewise. * typecheck/rust-hir-type-check-expr.h: Likewise. * checks/errors/rust-hir-pattern-analysis.cc (PatternChecker::visit (IfLetExpr &)): Remove. (PatternChecker::visit (IfLetExprConseqElse &)): Remove. * checks/errors/rust-hir-pattern-analysis.h (visit(IfLetExpr &)): Remove. (visit(IfLetExprConseqElse &)): Remove. gcc/testsuite/ChangeLog: * rust/compile/if_let_expr.rs: Adjust. * rust/compile/if_let_expr_simple.rs: New test. * rust/compile/iflet.rs: New test. * rust/execute/torture/iflet.rs: New test. * rust/compile/nr2/exclude: Add iflet.rs and if_let_expr_simple.rs Signed-off-by: Marc Poulhiès <[email protected]>

dkm self-assigned this Jun 23, 2024

dkm marked this pull request as draft June 23, 2024 21:58

CohenArthur reviewed Jun 24, 2024

View reviewed changes

gcc/rust/hir/rust-ast-lower-block.h Show resolved Hide resolved

dkm force-pushed the dkm/iflet_desugar branch 3 times, most recently from 54e06f0 to 19a5647 Compare July 2, 2024 21:06

dkm force-pushed the dkm/iflet_desugar branch from 19a5647 to 8726aae Compare August 28, 2024 09:05

dkm force-pushed the dkm/iflet_desugar branch 2 times, most recently from f8883da to 70b157e Compare September 5, 2024 20:06

dkm marked this pull request as ready for review September 5, 2024 20:08

P-E-P approved these changes Sep 10, 2024

View reviewed changes

gcc/rust/hir/rust-ast-lower.cc Outdated Show resolved Hide resolved

dkm mentioned this pull request Sep 17, 2024

Have a way to track down synthetic HIR from HIR lowered directly from source #3167

Open

dkm force-pushed the dkm/iflet_desugar branch 2 times, most recently from 9a345d4 to 07c54ea Compare September 21, 2024 20:04

dkm force-pushed the dkm/iflet_desugar branch 2 times, most recently from 491b07a to 7f1e406 Compare October 4, 2024 06:36

dkm mentioned this pull request Oct 6, 2024

Crash when using nr2 on a simple iflet expression #3189

Open

dkm force-pushed the dkm/iflet_desugar branch from 7f1e406 to 000f8eb Compare October 16, 2024 08:51

dkm requested a review from P-E-P October 16, 2024 15:45

P-E-P approved these changes Oct 28, 2024

View reviewed changes

dkm force-pushed the dkm/iflet_desugar branch from 000f8eb to 75ed46f Compare October 28, 2024 18:54

dkm added this pull request to the merge queue Oct 28, 2024

Merged via the queue into master with commit 5fe9110 Oct 28, 2024
12 checks passed

dkm deleted the dkm/iflet_desugar branch October 29, 2024 08:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Desugar IfLet* expr to match #3064

Desugar IfLet* expr to match #3064

dkm commented Jun 23, 2024 •

edited

Loading

dkm commented Sep 5, 2024

dkm commented Sep 6, 2024

P-E-P left a comment

dkm commented Sep 10, 2024

P-E-P commented Sep 11, 2024

dkm commented Sep 11, 2024

dkm commented Sep 12, 2024

dkm commented Oct 6, 2024

dkm commented Oct 15, 2024

P-E-P commented Oct 15, 2024 •

edited

Loading

dkm commented Oct 16, 2024

P-E-P left a comment

Desugar IfLet* expr to match #3064

Desugar IfLet* expr to match #3064

Conversation

dkm commented Jun 23, 2024 • edited Loading

dkm commented Sep 5, 2024

dkm commented Sep 6, 2024

P-E-P left a comment

Choose a reason for hiding this comment

dkm commented Sep 10, 2024

P-E-P commented Sep 11, 2024

dkm commented Sep 11, 2024

dkm commented Sep 12, 2024

dkm commented Oct 6, 2024

dkm commented Oct 15, 2024

P-E-P commented Oct 15, 2024 • edited Loading

dkm commented Oct 16, 2024

P-E-P left a comment

Choose a reason for hiding this comment

dkm commented Jun 23, 2024 •

edited

Loading

P-E-P commented Oct 15, 2024 •

edited

Loading